feat(kad): enable putting Record without publisher#6176
feat(kad): enable putting Record without publisher#6176skaunov wants to merge 4 commits intolibp2p:masterfrom
Record without publisher#6176Conversation
skaunov
changed the title
Record without publisherRecord without publisher
skaunov
changed the title
Record without publisherRecord without publisher
skaunov
changed the title
Record without publisherRecord without publisher
elenaf9
left a comment
elenaf9
left a comment
There was a problem hiding this comment.
Thanks for the PR!
I appreciate the general clean-up of the code, but it does make it hard to review. Given that I currently have limited time for reviews in general, I'd prefer it you could revert unrelated changes unless you feel strongly about them.
| pub fn new<K>(key: K, value: Vec<u8>, publisher: Option<PeerId>) -> Self | ||
| where | ||
| K: Into<Key>, | ||
| { | ||
| Record { | ||
| key: key.into(), | ||
| value, | ||
| publisher: None, | ||
| publisher, | ||
| expires: None, | ||
| } |
There was a problem hiding this comment.
I would prefer if we don't break the API and instead add a new method Record::new_anonymous or similar. And then just set a dummy Id here or sth like that, since the actual ID is replaced in Behavior::put_record anyway.
There was a problem hiding this comment.
I see what you want. I'd say ("for the record", the pun unintended) I really disagree with the direction, but I don't think my (obvious?) reasons are interesting to anybody now; and it's kinda problems of those who will disentangle the logic the days after. So I can go with it to get anonymous Record upstream. 👿
On the second thought: if instead of a dummy, separate the (new) logic of put_record between new and new_anonymous the change will be not degrading and even more minimal (add new_anonymous and remove the line from put_record).
| // single request-response style exchange with a single remote peer. A query | ||
| // is made of multiple requests across multiple remote peers. | ||
| InboundRequest { request: InboundRequest }, | ||
| InboundRequest(InboundRequest), |
There was a problem hiding this comment.
Please revert this change and the one below. We should avoid breaking the API unless it is necessary.
|
On Thu, Oct 9 2025 at 11:59:49 AM -0700, Elena Frank ***@***.***> wrote:
I appreciate the general clean-up of the code, but it does make it
hard to review. Given that I currently have limited time for reviews
in general, I'd prefer it you could revert unrelated changes unless
you feel strongly about them.
That's understandable! No strong feeling for those changes: just that
clean-up. Let me minimize the impact; I guess I'll do it tomorrow, and
will close this one in spite of that minimized -- just in case someone
will start to help you with the reviews and the fresh enthusiasm could
reach this far. X)
|
…ing way. Sorry for that, see <libp2p#6176 (comment)> for some details.
2 participants
Description
The change is about allowing putting
Recordwithpublisher: Nonesacrificing republishing.Sorry for so many lateral changes: I had to delve deeper to understand that the change would actually work and during active reading was eliminating redundant pieces which confuse comprehension with misleading supposition (looking for further use of a variable when it is actually just a parameter/argument was most common impediment).
Notes & open questions
I don't have a fine test for this in mind as it doesn't really do anything (because replication context doesn't yield an event). Simultaneously I'm not really satisfied with the approach I took to adapt/fix the test (retaining only
Recordwithpublisher: Some); but I need a hint if that's possible to do better due to the same fact of how silent replication is.Change checklist